Skip to content

Gate reader discovery on cped_slot rather than undefined_addr - #403

Open
szegedi wants to merge 1 commit into
mainfrom
szegedi/thread-ctx-liveness
Open

Gate reader discovery on cped_slot rather than undefined_addr#403
szegedi wants to merge 1 commit into
mainfrom
szegedi/thread-ctx-liveness

Conversation

@szegedi

@szegedi szegedi commented Aug 28, 2026

Copy link
Copy Markdown

Makes cped_slot the field that the reader checks before it dereferences anything, and orders the writes to the thread local structure around it so it's written last on construction and cleared first on destruction.

I'm in the process of writing an OTEP specification for the Node.js thread context protocol, so I'm ironing out some wrinkles I'm coming across with observable ordering of writes etc.

Why cped_slot rather than undefined_addr

Our initial recommendation was to check undefined_addr not being zero for this, but it was actually serving two purposes: the value a reader compares against to detect "no context attached", and the liveness flag.

I decided to instead use cped_slot for the liveness, for the following reasons:

  • It's the first pointer the reader needs to dereference, so it makes sense for it to act as the gate. cped_slot != 0 says "there is a slot to read", and undefined_addr != 0 says "someone initialized this struct at some point".
  • The two already disagree on Node < 22, where cped_slot remains null because that V8 has no CPED but undefined_addr is set to non-zero. (We keep using undefined_addr != 0 as our internal "someone initialized this struct" signal, though.)

The ordering

This really started out as an a-ha moment of "if undefined_addr is used by the reader as a gate, then I must make sure its initialization order is guaranteed", which then led me to "wait, why are we using undefined_addr and not cped_slot for this?".

And to boot, the order for undefined_addr gate was actually wrong, as it was cleared last so it'd still be nonzero in front of other already-cleared fields and the reader can stop the thread while this is happening and observe the inconsistency. This was harmless in practice only because the cleared fields are null and cross-process reads of address 0 fail, so… safety by accident.

Both stores are now volatile behind an atomic_signal_fence and ordered to happen correctly.

@github-actions

Copy link
Copy Markdown

Overall package size

Self size: 2.55 MB
Deduped: 3.26 MB
No deduping: 3.26 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Aug 28, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 7 Pipeline jobs failed

DataDog/apm-reliability/pprof-nodejs | benchmarks: [18]

View more details · View in GitLab

DataDog/apm-reliability/pprof-nodejs | benchmarks: [20]

View more details · View in GitLab

DataDog/apm-reliability/pprof-nodejs | benchmarks: [22]

View more details · View in GitLab

View all 7 failed jobs.

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 5b03b0e | Docs | View more details | Give us feedback!

@szegedi
szegedi force-pushed the szegedi/thread-ctx-liveness branch from 3aab0d0 to 5b03b0e Compare August 28, 2026 12:07
@szegedi szegedi added the semver-patch Bug or security fixes, mainly label Aug 28, 2026
@szegedi
szegedi marked this pull request as ready for review August 28, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch Bug or security fixes, mainly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant